-
Notifications
You must be signed in to change notification settings - Fork 313
feat: Refactor prometheus metrics #1243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
49b6eca to
20f13ee
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1243 +/- ##
==========================================
+ Coverage 70.34% 70.55% +0.21%
==========================================
Files 50 49 -1
Lines 4461 4449 -12
==========================================
+ Hits 3138 3139 +1
+ Misses 1130 1117 -13
Partials 193 193 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ee686a1 to
2d26552
Compare
|
For the metric port and health port flags, shall we also support setting them via env var? If so, also update The new flag names, health-probe-bind-address, metrics-bind-address, look like support both hostname or ip plus port number. But I think in practice, we only use the port number part. What are the accepted format, are values like :9999 or 9999 both acceptable? I'm wondering if we should rename them from the old health-port and metrics-port, or just stick to the old names. The old names are consistent with argo-cd (for example, https://github.com/argoproj/argo-cd/blob/master/cmd/argocd-server/commands/argocd_server.go#L314), and a bit more readable. If we only care about the port part, I feel the old names are fine. |
cmd/run.go
Outdated
| // TODO: flags below are not documented yet and don't have env vars yet. Metrics and health checks will be implemented in GITOPS-7113 | ||
| controllerCmd.Flags().StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.") | ||
| controllerCmd.Flags().StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to.") | ||
| controllerCmd.Flags().StringVar(&probeAddr, "health-probe-bind-address", ":8081", "The address the probe endpoint binds to. Leave as 0 to disable the probe service.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the default is :8081, so 'Leave as 0' can be confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the health-probe-bind-address work with the liveness setting in manager.yaml? Are there overlapping, or should they be kept in sync?
livenessProbe:
httpGet:
path: /healthz
port: 8081There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They should be kept in sync. If these settings are not synchronized, the health check will fail, leading to the pod being terminated and restarted. health-probe-bind-address tells the controller which port to listen on for incoming health check requests. livenessProbe is a k8s setting that tells the kubelet where to send requests to check if the container is healthy. If they don't match the health check fails, connection is refused and k8s kills the pod.
We didn't have env vars before for these flags, argocd doesn't have env vars for metrics port (see link you provided), metrics are optional and disabled by default (it's |
The accepted formats are: The difference between argocd flags and image updater flags is that argocd flag is health-probe-bind-address, metrics-bind-address flag names were autogenerated by kubebuilder. |
Co-authored-by: Gemini AI <[email protected]>" Signed-off-by: Denis Karpelevich <[email protected]>
2d26552 to
a7a7456
Compare
Prometheus metrics were successfully exported after refactoring
health-portandmetrics-portand replace them withhealth-probe-bind-addressandmetrics-bind-address. Update docs.healthmodule was removed in favor of controller-runtime.